Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: retain user RPC timeout if set via withTimeout #1324

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Mar 9, 2021

If a Callable has RetrySettings and the calculated RPC Timeout for an attempt is less than the constant, RPC timeout provided by the user via ApiCallContext.withTimeout, the attempt settings will win. This is not caller friendly.

This PR changes AttemptCallable, used for Unary RPCs, only attempts to set the ApiCallContext.withTimeout when it is not already set by the caller.

This was fixed for ServerStreaming RPCs in #1155

Addresses commentary in #1144.

@noahdietz noahdietz requested review from a team as code owners March 9, 2021 01:36
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 9, 2021
@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #1324 (c26cac6) into master (27d92c6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1324   +/-   ##
=========================================
  Coverage     79.65%   79.65%           
- Complexity     1248     1249    +1     
=========================================
  Files           209      209           
  Lines          5461     5461           
  Branches        464      464           
=========================================
  Hits           4350     4350           
  Misses          928      928           
  Partials        183      183           
Impacted Files Coverage Δ Complexity Δ
...n/java/com/google/api/gax/rpc/AttemptCallable.java 82.60% <100.00%> (ø) 5.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27d92c6...c26cac6. Read the comment docs.

@noahdietz
Copy link
Contributor Author

@elharo do you want to take another pass or am I OK to merge?

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to wait for me. If it's approved, you can merge

@noahdietz noahdietz merged commit 3fe1db9 into googleapis:master Mar 10, 2021
@noahdietz noahdietz deleted the retain-with-timeout branch March 10, 2021 23:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants